Skip to content

Conversation

alarso16
Copy link
Contributor

Why this should be merged

Works to follow avalanchego's linter whenever possible. Some of the ErrorContains really tested geth code, making it difficult to remove.

How this works

Uses sentinel errors and nolint clauses.

How this was tested

Existing UT

Need to be documented?

No

Need to update RELEASES.md?

No

@alarso16 alarso16 force-pushed the alarso16/error-contains branch from 38af157 to 1734bc4 Compare September 30, 2025 13:34
ctx: ctx,
rules: vmtest.ForkToRules(upgradetest.NoUpgrades),
expectedErr: "atomic input failed verification",
expectedErr: avax.ErrNilTransferableInput,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the specific error we were checking, but I'm not sure it's good practice to rely on an error "so far away"

}
} else {
switch {
case ethHeader.ParentBeaconRoot != nil:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only moved because the test fails for a different error than what it was intending to check.

@alarso16 alarso16 self-assigned this Sep 30, 2025
@alarso16 alarso16 marked this pull request as ready for review September 30, 2025 14:52
@alarso16 alarso16 requested a review from a team as a code owner September 30, 2025 14:52
@JonathanOppenheimer
Copy link
Member

Just a question (and I'll do an actual review) - I remember talking about the vmerrorsin terms of the uplift - do we want to use that package for the sentinel errors declared? Do we want to get rid of the package entirely?

@alarso16
Copy link
Contributor Author

Just a question (and I'll do an actual review) - I remember talking about the vmerrorsin terms of the uplift - do we want to use that package for the sentinel errors declared? Do we want to get rid of the package entirely?

That package is used to share errors between packages to avoid circular dependencies - for the errors I added, I didn't actually need to move them, but I definitely could. I think the package is necessary for the existing errors though

@JonathanOppenheimer
Copy link
Member

Just a question (and I'll do an actual review) - I remember talking about the vmerrorsin terms of the uplift - do we want to use that package for the sentinel errors declared? Do we want to get rid of the package entirely?

That package is used to share errors between packages to avoid circular dependencies - for the errors I added, I didn't actually need to move them, but I definitely could. I think the package is necessary for the existing errors though

I see - I didn't realize it was just for errors that would create circular dependencies!

@JonathanOppenheimer
Copy link
Member

By the way the vmwrrors package should be can be removed entirely -- obviously not for this PR but if you take a look at my uplift PR Josh explained how.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants